Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[12.0] Add .gitattributes for merge automatically OCA_dependencies.txt file #243

Closed
wants to merge 1 commit into from

Conversation

jcoux
Copy link

@jcoux jcoux commented Jan 10, 2020

No description provided.

@rousseldenis
Copy link
Contributor

@jcoux @gurneyalex I'm not so much in favor of this as it could lead to duplicate entries (e.g.: two lines for sale-workflow and a branch on it if contributor added it to make PR green).

Moreover, if we do so, we should generalize it.

@yvaucher
Copy link
Member

Just one remark there, this will be useful only in case of manual merging, github doesn't do it automatically. Not sure about the OCA bot. Maybe something to define on the OCA bot?

We could sort this out with a little more complexe merge policy. As merge policy could be extended with scripts. We would need something like add all, sort and keep unique.

@sbidoul any opinion on this, do you think it's the role of the ocabot try to make the rebase on it's own? Thus needing us to give it strong directives?
Personally I would say that as long as we can identify something that will be repeated often and for which we identify the workflow/boundaries we want to automate it.

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2020

The ocabot merge strategy could be improved to cope with that, but...

I still think oca_dependencies.txt is redundant, as well as requirements.txt because all dependencies are expressed in the manifests. And we can easily get rid of both of them by installing addons to test with pip: OCA/maintainer-quality-tools#343

So I'd rather address the root cause of the issue (redundant files) than add automation complexity to cope with them.

Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the incentive here was to be able to merge multiple pending merge in local project without the hassle of having an unnecessary conflict when adding various PR that adds modules that each adds dependencies.

So in the mean time I'm ok with this change

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@sbidoul
Copy link
Member

sbidoul commented Oct 1, 2020

@yvaucher FYI, installation of dependencies is now implemented in MQT. It is opt-in: to activate it you need to set MQT_DEP=PIP in .travis.yml environment variables.

@yvaucher yvaucher closed this Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants